Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

native upcasts #113

Merged
merged 12 commits into from
Sep 18, 2023
Merged

native upcasts #113

merged 12 commits into from
Sep 18, 2023

Conversation

matthiasdiener
Copy link
Contributor

@matthiasdiener matthiasdiener commented Aug 28, 2023

Is this the approach you had in mind @inducer? (the actual code to create the upcasts would be added to gen_wrap.py)

1. Substitute for make_new_upcast_wrapper:

Option a)
  wrap_basic_set.def("foreach_point", [](isl::basic_set &self, py::object fn) 
                     { return isl::set_foreach_point(self, fn); });
Option b)
  wrap_basic_set.def("foreach_point", isl::set_foreach_point);

(I think the actual cast is handled by impicitly_convertible)

2. Substitute for make_existing_upcast_wrapper:

Option a)
wrap_basic_set.def("union", [](isl::basic_set &self, isl::union_set &arg_set2)
                                { return isl::union_set_union(self, arg_set2); });
Option b)
wrap_basic_set.def("union", isl::union_set_union);

Edit: I implemented option b) for both cases.

@inducer
Copy link
Owner

inducer commented Aug 29, 2023

Yes, something like that would work.

@matthiasdiener matthiasdiener force-pushed the native-upcast branch 2 times, most recently from 1abb955 to e0cfea1 Compare August 29, 2023 20:48
@matthiasdiener
Copy link
Contributor Author

When built against nanobind (+ #116), this provides a modest speedup for the microbenchmark on M1:

@inducer
Copy link
Owner

inducer commented Sep 12, 2023

I'm OK with option b) in both cases. I'm not sure option a) would work unmodified. You would likely need to call the casting function manually somewhere in there in order to get it to work, and this may be incrementally faster than option b). But a lot depends on exactly how much faster, and whether that justifies the extra effort (and generated code).

At the same time, there's a small amount of speed-up here, and we're net deleting code: I'd call it a win regardless and would be happy to merge this, I think.

@matthiasdiener
Copy link
Contributor Author

I'm OK with option b) in both cases. I'm not sure option a) would work unmodified. You would likely need to call the casting function manually somewhere in there in order to get it to work, and this may be incrementally faster than option b). But a lot depends on exactly how much faster, and whether that justifies the extra effort (and generated code).

Option a) works in the way it is written above (possibly also due to making use of implicitly_convertible?). Both options are very close performance-wise in my test.

@matthiasdiener matthiasdiener marked this pull request as ready for review September 13, 2023 18:59
@inducer inducer merged commit 66e0f1c into inducer:main Sep 18, 2023
15 checks passed
@inducer
Copy link
Owner

inducer commented Sep 18, 2023

Thanks!

@matthiasdiener matthiasdiener deleted the native-upcast branch September 18, 2023 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants